[FC-0099] refactor!: consider global scope as wildcard managed by ScopeData#132
[FC-0099] refactor!: consider global scope as wildcard managed by ScopeData#132mariajgrimaldi merged 8 commits intomainfrom
Conversation
|
Thanks for the pull request, @mariajgrimaldi! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
| # The 'sc' namespace is used for generic scopes that aren't tied to a specific resource type. | ||
| # This base class supports: | ||
| # 1. Global scopes (external_key='*') that apply across all resource types | ||
| # 2. Custom generic scopes that don't map to specific domain objects | ||
| # Subclasses like ContentLibraryData ('lib') represent concrete resource types. | ||
| NAMESPACE: ClassVar[str] = "sc" |
There was a problem hiding this comment.
@bmtcril: remember we talked about the usage of this generic scope? I think we found one!
I was thinking of changing NAMESPACE to an empty string cause I'm not sure how descriptive is sc. What do you think?
FYI @MaferMazu @BryanttV
There was a problem hiding this comment.
Maybe we could change it to something related to "anonymous", as if it's supposed to identify scopes that don't map to specific objects, then they are "anonymous"?
Thinking on the usage of a namespaced key, it would look something like:
"anon^*"
Otherwise if we use an empty string, it woud be like "^*" right?
Won't this cause confusion because of the "missing" namespace?
There was a problem hiding this comment.
sc means scope 🥲, not very explicit though. Initially was only to have base definitions for all the other scopes, but now we can use it for global scopes so it might make sense to change this namespace.
Won't this cause confusion because of the "missing" namespace?
I think it will! Considering what I said here, https://github.com/openedx/openedx-authz/pull/132/files#r2486208488, instead of anonymous, should we use global or some synonym?
There was a problem hiding this comment.
I think global makes the most sense, but whatever we use needs to be something that will never get a real value of the same name, right?
There was a problem hiding this comment.
Exactly! What about these?
- global:
global^*(++1 most votes) - instance:
instance^*
In all cases, having something like can_create_libraries in NAMESPACE^* would mean I can create any number of libraries in my Open edX instance. This only makes sense for the * external key, right? It wouldn’t make sense for a specific key or would it also apply there? If so, then maybe we won't get values with the same name?
There was a problem hiding this comment.
My understanding here is that in the case of the can_create_libraries permission, global^* Would mean that it applies to anything, not only libraries, but in practice can_create_libraries will only be evaluated on lib objects.
But, if at some point we add some more generic permissions, like can_create, that could apply that more than one domain object, then we could define more specific things, like global^*:DemoX:CSPROB, that would apply on anything on the org DemoX that is called CSPROB (courses, libs, etc). But this is a whole different level of complexity that I don't think we need now.
There was a problem hiding this comment.
The thing with can_create_libraries is that we can't actually evaluate it with a concrete library, but only against the instance (not sure how to call it) so it could be similar to can_create but only checked in context of libraries - hopefully I'm not getting all of this mixed up (so please correct me if I'm wrong!) 😅
that would apply on anything on the org DemoX that is called CSPROB (courses, libs, etc). But this is a whole different level of complexity that I don't think we need now.
I totally agree. Do you think Oh I understand now! We should consider also cases without the global^*:DemoX:CSPROB wouldn't work for that can_create case?*!
There was a problem hiding this comment.
Oh you are right, for can_create_libraries it doesn't make sense to specify a concrete library, so global makes sense there. It would make sense for something like can_edit_library instead.
| # an external_key to mean generic scope which maps to base ScopeData class. | ||
| # The only remaining issue is that internally the namespace key used in policies will be | ||
| # The generic scope namespace (sc^*), so we need to handle that case here. | ||
| if kwargs.get("external_key") == GENERIC_SCOPE_WILDCARD: |
There was a problem hiding this comment.
Generic or global?
There was a problem hiding this comment.
If this is a question on naming, I think global is clearer.
There was a problem hiding this comment.
I initially used generic scope cause this scope can be specialized into other scopes: content libraries, courses... But this might be too ambiguous. It might make sense making it global when not specialized?
There was a problem hiding this comment.
Global makes more sense to me 👍
| # an external_key to mean generic scope which maps to base ScopeData class. | ||
| # The only remaining issue is that internally the namespace key used in policies will be | ||
| # The generic scope namespace (sc^*), so we need to handle that case here. | ||
| if kwargs.get("external_key") == GENERIC_SCOPE_WILDCARD: |
There was a problem hiding this comment.
If this is a question on naming, I think global is clearer.
| # The 'sc' namespace is used for generic scopes that aren't tied to a specific resource type. | ||
| # This base class supports: | ||
| # 1. Global scopes (external_key='*') that apply across all resource types | ||
| # 2. Custom generic scopes that don't map to specific domain objects | ||
| # Subclasses like ContentLibraryData ('lib') represent concrete resource types. | ||
| NAMESPACE: ClassVar[str] = "sc" |
| # The 'sc' namespace is used for generic scopes that aren't tied to a specific resource type. | ||
| # This base class supports: | ||
| # 1. Global scopes (external_key='*') that apply across all resource types | ||
| # 2. Custom generic scopes that don't map to specific domain objects | ||
| # Subclasses like ContentLibraryData ('lib') represent concrete resource types. | ||
| NAMESPACE: ClassVar[str] = "sc" |
There was a problem hiding this comment.
Maybe we could change it to something related to "anonymous", as if it's supposed to identify scopes that don't map to specific objects, then they are "anonymous"?
Thinking on the usage of a namespaced key, it would look something like:
"anon^*"
Otherwise if we use an empty string, it woud be like "^*" right?
Won't this cause confusion because of the "missing" namespace?
Co-authored-by: Rodrigo Mendez <[email protected]>
|
@MaferMazu: can you confirm this version still works for you? |
There was a problem hiding this comment.
Should we change this to GLOBAL_SCOPE_WILDCARD?
There was a problem hiding this comment.
Done! Thanks for the suggestion :)
bmtcril
left a comment
There was a problem hiding this comment.
I think this is good to go after the change of GENERIC_SCOPE_WILDCARD to GLOBAL_SCOPE_WILDCARD.
MaferMazu
left a comment
There was a problem hiding this comment.
Thanks for the changes @mariajgrimaldi, they work as expected <3
Description
This PR adds support to the global type by allowing the use of "*" as a global external_key that will be managed by the base ScopeData class with
globalnamespace.Merge checklist:
Check off if complete or not applicable: